• I thought about documentation and added the doc-change-required label to this PR if updates are required.
• I thought about the changelog and included a changelog update if required.
• If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@fab-10 would you be happy to put a review on the PR?

@macfarla , @pinges - would one of you be able to take a look at this as well to see what you think?

The devp2p discv4 docs for PING (https://github.com/ethereum/devp2p/blob/master/discv4.md#ping-packet-0x01) seem to only say that it's a field in the packet:

packet-data = [version, from, to, expiration, enr-seq ...]
version = 4
from = [sender-ip, sender-udp-port, sender-tcp-port]
to = [recipient-ip, recipient-udp-port, 0]

and that the recipient should send a PONG packet in reply, without being specific about where to reply to:

When a ping packet is received, the recipient should reply with a [Pong](https://github.com/ethereum/devp2p/blob/master/discv4.md#pong-packet-0x02) packet. It may also consider the sender for addition into the local table. Implementations should ignore any mismatches in version.

The docs for PONG (https://github.com/ethereum/devp2p/blob/master/discv4.md#pong-packet-0x02) don't seem to say much about where a PONG packet has to reply to either:

Pong is the reply to ping.

ping-hash should be equal to hash of the corresponding ping packet. Implementations should ignore unsolicited pong packets that do not contain the hash of the most recent ping packet.

The enr-seq field is the current ENR sequence number of the sender. This field is optional.

Interestingly the ethereum discovery overview wiki (https://github.com/ethereum/devp2p/wiki/Discovery-Overview#discovery-v4) says this:

Certain specified packet fields such as the "From" field in a Ping are ignored by implementations.

It's not completely clear to me whether that means "must be ignored by" or "are often ignored by"? If it's the case that it should be ignored (and that this PR/issue isn't valid) I'm a bit confused as to the purpose of the --p2p-host option.

With your change I think we would run in to problems with nodes that do not correctly set the FROM field in the PING message.

Yes I agree, it's difficult to know if fixing this is going to break existing setups that rely on the current behaviour.

I think running a mainnet node for a period of time like you describe would be a useful way of getting some more data to base a decision on. I'm not sure if I'm able to do that personally (I need to look into what I have available re. a public-facing system) but I think it would be a useful test.

I also wonder if there's any convention within Besu for a "fallback to previous behaviour" env-var or similar (something that doesn't require a first-class config option, but gives any user a way of reverting to the old behaviour if their environment doesn't suit the new behaviour). I think a first-class config option (e.g. something like --p2p-honor-from-addr) is probably too heavy weight just to provide a way of reverting the behaviour, particularly if the consensus that the new behaviour is correct.

Finally, maybe a look at what options Geth & Nethermind offer and what they do in code with that field would be useful input to the discussion?

For Nethermind, based on NethermindEth/nethermind...PingMsgSerializer, I think they honor the From field of the message. For go-ethereum, it's a bit more tricky to say b/c the deserialization occurs in ethereum/go-ethereum...v4wire.go but using reflection forgery to match the order in the rlp list with the position of the fields of every struct but, in a nutshell, I think they also use the From from the packet

That's really useful context @diega

@pinges - do you think based on that we would be comfortable releasing the change in behaviour without an option to disable/revert to the old behaviour?

@pinges, wondered if you had any more thoughts based on the comment from @diega?

Merging for now as this would appear to be in keeping with how other clients work.

I see a huge amount of error message after merging main banch today . Is it related to this PR ?

{"@timestamp":"2024-01-19T11:31:39,831","level":"ERROR","thread":"vert.x-eventloop-thread-3","class":"ContextBase","message":"Unhandled exception","throwable":" java.lang.IllegalArgumentException: Invalid ip address.\n\tat org.hyperledger.besu.ethereum.p2p.peers.EnodeURLImpl$Builder.ipAddress(EnodeURLImpl.java:405)\n\tat org.hyperledger.besu.ethereum.p2p.peers.EnodeURLImpl$Builder.ipAddress(EnodeURLImpl.java:380)\n\tat org.hyperledger.besu.ethereum.p2p.discovery.PeerDiscoveryAgent.handleIncomingPacket(PeerDiscoveryAgent.java:312)\n\tat org.hyperledger.besu.ethereum.p2p.discovery.VertxPeerDiscoveryAgent.lambda$handlePacket$10(VertxPeerDiscoveryAgent.java:298)\n\tat io.vertx.core.impl.future.FutureImpl$3.onSuccess(FutureImpl.java:141)\n\tat io.vertx.core.impl.future.FutureBase.lambda$emitSuccess$0(FutureBase.java:54)\n\tat io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:173)\n\tat io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:166)\n\tat io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470)\n\tat io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:413)\n\tat io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)\n\tat io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)\n\tat io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)\n\tat java.base/java.lang.Thread.run(Thread.java:1583)\n"}

Just having a look @matkt. Is this syncing a public node?

yes during checkpoint on mainnet, directly at the beginning

Just started a thread on discord if it's easier to discuss on there? https://discord.com/channels/905194001349627914/905205502940696607/1197868266635407410

I've raised #6439 which validates the From address as a valid IP before attempting to use it, and reverts to the previous behaviour of taking it from the UDP source IP if it is invalid.